Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[stdlib] make cookies module modern #17116

Merged
merged 12 commits into from
Feb 24, 2021
Merged

[stdlib] make cookies module modern #17116

merged 12 commits into from
Feb 24, 2021

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Feb 20, 2021

Ref #10604

std/cookies is not good enough, people are keeping creating a new one.

https://github.com/dom96/jester/blob/master/jester/private/utils.nim
https://github.com/planety/cookiejar
https://github.com/itsumura-h/nim-basolato/blob/master/src/basolato/core/utils.nim
https://github.com/achesak/nim-biscuits#about

BTW Python has deprecated cgi module at Python 3.8 and will remove cgi module at Python 3.10.

93.25% of browsers support sameSite attribute.

This feature is backwards compatible. Browsers not supporting this feature will simply use the cookie as a regular cookie. There is no need to deliver different cookies to clients.

https://caniuse.com/?search=sameSite

lib/pure/cookies.nim Outdated Show resolved Hide resolved
@ringabout
Copy link
Member Author

ringabout commented Feb 20, 2021

see also https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie/SameSite

Standards related to the Cookie SameSite attribute recently changed such that:
The cookie-sending behavior if SameSite is not specified is SameSite=Lax. Previously the default was that cookies were sent for all requests.
Cookies with SameSite=None must now also specify the Secure attribute (they require a secure context/HTTPS).

https://tools.ietf.org/html/draft-west-cookie-incrementalism-00

@ringabout ringabout changed the title [stdlib] update cookies module [stdlib] make cookies module modern Feb 22, 2021
lib/pure/cookies.nim Outdated Show resolved Hide resolved
@ringabout ringabout requested a review from dom96 February 23, 2021 12:33
@Araq Araq merged commit 46bd222 into nim-lang:devel Feb 24, 2021
SameSite* {.pure.} = enum ## The SameSite cookie attribute.
## `Default` means that `setCookie`
## proc will not set `SameSite` attribute.
Default, None, Lax, Strict
Copy link
Member

@timotheecour timotheecour Feb 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std/compilesettings uses lower-case enum values, this is inconsistent; we should specify in nep1 which one is the "best practice"
followup: timotheecour#622 (comment)

Copy link
Member Author

@ringabout ringabout Feb 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is already specified in nep1: Non-pure enum values should use camelCase whereas pure enum values should use PascalCase.

image

## Creates a command in the format of
## ``Set-Cookie: key=value; Domain=...; ...``
return setCookie(key, value, domain, path,
## `Set-Cookie: key=value; Domain=...; ...`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-existing but this overload (with differently placed params) seems like a bad design, instead we should expose a proc (eg toGmtString(t: DateTime|Time) and let users call:

setCookie(..., t.toGmtString, ...)

ringabout added a commit to ringabout/Nim that referenced this pull request Mar 22, 2021
* update cookies module
* introduce sameSite.Default

Co-authored-by: hlaaftana <[email protected]>
ardek66 pushed a commit to ardek66/Nim that referenced this pull request Mar 26, 2021
* update cookies module
* introduce sameSite.Default

Co-authored-by: hlaaftana <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants